-
Notifications
You must be signed in to change notification settings - Fork 4.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update for MediaPlaceholder component: don't stack error messages when re-uploading #14721
Update for MediaPlaceholder component: don't stack error messages when re-uploading #14721
Conversation
Hello @noisysocks, taken into account your feedback from here: #10250 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR, it works great in the audio block. We should do the same in the other blocks with upload option: Gallery, Media & Text, Video, Cover, File.
@@ -81,6 +92,8 @@ class MediaContainer extends Component { | |||
onSelect={ onSelectMedia } | |||
accept="image/*,video/*" | |||
allowedTypes={ ALLOWED_MEDIA_TYPES } | |||
notices={ noticeUI } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @Soean for taking time and reviewing this. |
@gziolo, may I also have your review on this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your patience in the review here. While there's separate ongoing discussion about how withNotices
should be implemented or if it should be used (e.g. #9442), this seems like a sensible improvement on the current iteration of notices use in these components.
const { noticeOperations } = this.props; | ||
noticeOperations.removeAllNotices(); | ||
noticeOperations.createErrorNotice( message ); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The spaces here ought to be a tab.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed. thx.
Thanks for letting me know this, I've subscribed to that discussion and related PRs in order to be on the same page and to apply the final solution when it will be available. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the work here. Tested across all of the impacted blocks and appears to work well 👍
Feedback addressed (applied to requested blocks)
Description
Fixes: #6106
The implementation is based on this stale PR's feedback: #10250
How has this been tested?
It was tested locally.
Screenshots
Checklist: